Skip to content

New custom property support - #2972

Open
raviks789 wants to merge 241 commits into
masterfrom
dictionary-support
Open

New custom property support#2972
raviks789 wants to merge 241 commits into
masterfrom
dictionary-support

Conversation

@raviks789

@raviks789 raviks789 commented Apr 1, 2025

Copy link
Copy Markdown
Collaborator

Motivation

Icinga 2 supports dictionaries as native variable type, but Director had not completely supported the dictionary type custom variables. Operators using service apply-for rules to generate services from dictionary type host custom variables were forced to define those variables outside Director entirely, losing managed configuration. This PR closes both gaps together: Director now supports a dictionary type custom variable and service apply-for rules can iterate over them directly.

Changes

  • Model: A new DirectorProperty model with explicit types: string, number, boolean, sensitive, fixed/dynamic arrays, strict/non-strict datalist bindings, and fixed (single level)/dynamic (nested) dictionaries.
  • Config: Dictionaries and other custom variable types render to valid Icinga 2 DSL. Apply-for rules on services now work correctly when the source variable is an array or dynamic dictionary, with the iteration key exposed as a first-class selector.
  • Migration: A migration command converts existing datafields to new custom variables, with --dry-run and --delete options. The export command includes custom variable definitions.
  • Basket : Custom variable definitions and per object assignments round-trip correctly through basket snapshots, including UUID relinking for dictionary parent-child relationships.
  • UI: A dedicated section for defining custom variable configuration. And a new object tab to add and assign custom variable values for the objects.

@cla-bot cla-bot Bot added the cla/signed label Apr 1, 2025
@raviks789
raviks789 force-pushed the dictionary-support branch 9 times, most recently from dd916d4 to f19501d Compare April 2, 2025 10:43
@raviks789
raviks789 force-pushed the dictionary-support branch 2 times, most recently from 2ddc75b to 3583514 Compare April 11, 2025 15:55
@raviks789
raviks789 force-pushed the dictionary-support branch 10 times, most recently from e818e3e to 3260c35 Compare May 14, 2025 15:29
@raviks789
raviks789 force-pushed the dictionary-support branch 8 times, most recently from 01bbc41 to e1fac6d Compare May 27, 2025 11:27
raviks789 added 10 commits July 28, 2026 09:44
MariaDB rejects ON UPDATE CASCADE on a column that a stored generated
column depends on, and parent_uuid_v depends on parent_uuid. Since uuid
primary keys never change, the clause was dead weight anyway. Dropped
it on both engines, kept ON DELETE CASCADE.
Restore recreates the property before we delete the category, so
cleanup needs to wipe that property first or the delete hits a FK error.
fixed-array, fixed-dictionary and dynamic-dictionary can no longer
nest inside another container since f051dc4. These tests still built
that shape and always throw now. The behavior they meant to cover is
already proven by the single-level tests that stay.
The dropdown already hid sensitive as a datalist item type since
those render every entry in the clear same as dynamic-array, but
beforeStore only checked for a dynamic-array parent. A basket restore
or direct write could still sneak one in. Added the datalist check
plus a test for both datalist kinds.
Every other custom variable type had a create-and-reload test except
bool, it only ever showed up as a value inside form fixtures. Filling
that gap.
The dynamic-array self-nesting rule was called out explicitly, but the
same limit for fixed-array was only implied by the general ban on
nesting container types. Made it explicit for symmetry.
…ay or datalist

The child side check only ran when the child got stored, so switching an
existing dictionary to a dynamic-array or datalist skipped it and left
a sensitive field exposed once rendered.
Checks both the array and datalist switch paths get rejected, and that
switching type still works fine when there is nothing sensitive to protect.
…ation comment

Existing PostgreSQL installations don't have citext enabled, only fresh
installs were told to. Migration 193 needs it for director_property.key_name,
so install it as the first statement of both the migration and the fresh
schema, ahead of any other DDL. citext is a trusted extension since Postgres
13, so this needs no manual step there; on older Postgres it fails cleanly
before anything else runs, so a retry after installing it by hand just works.
Masking returned a plain string, but a couple of spots handed that
straight into an HtmlElement which needs real markup, so it threw and
the whole property silently fell back to showing raw values. A schema
field named the same as its own parent dictionary also confused the
array vs dictionary check, causing the same crash. Also restored the
item count on dictionary rows at the root level, since it went missing
compared to arrays.
@flourish86

Copy link
Copy Markdown
Contributor

as requested by @raviks789, here's a design suggestion for integrating the "Required in inheriting objects".

boxed version with clearer and more consistent structure

Frame 5

Alternative suggestion without boxes

Frame 6

raviks789 added 12 commits July 29, 2026 13:44
key_name is case-insensitive in the new schema, but the legacy varname
column is not, so two datafields like region and Region never got
flagged as duplicates and both got queued for migration. The second
insert then failed on the unique constraint and rolled back the whole
batch, not just the two colliding fields. Same problem existed for the
existing-property check, which used a plain PHP array lookup instead
of comparing names the way the database actually does. Using
mb_strtolower instead of strtolower to keep this correct for non-ASCII
names too.
A POST that only carries vars, no other object properties, got its
status decided before the variable applier even ran, so it always
came back 304 even when it really changed data. CustomVariableValueApplier
now reports back whether it actually wiped or wrote anything, checked
against its own modification tracking before storeToDb resets it, and
the handler upgrades a stale 304 using that real signal instead of
guessing from the object's own columns alone.
It set loadedFromDb by hand but never touched loadedProperties, so
setProperties on a row loaded this way crashed. Use setBeingLoadedFromDb
instead.
Two instances can create the same root property with different uuids.
Restoring one into the other found the existing row by key_name, then
compared it against the incoming data before touching it. That
comparison always failed since uuid and items never lined up, so it
fell through to create() and hit the unique key_name constraint. Now
it just adopts the incoming values onto the existing candidate.
The summary counted total datafields after migrateDatafields() ran,
but --delete already removes migrated rows from director_datafield
by then. Snapshot the count before migration touches anything so the
skipped total stays correct.
…ill owns

deleteStoredValues() cleared icinga_*_var rows by varname alone. Migration
skips a datafield when a same-named property already exists, so both can
end up sharing one stored value. Deleting or retyping the property then
destroyed data the legacy field still relies on. Skip the wipe when a Data
Field with that varname is still around.
deleteStoredValues() silently kept values alive when a legacy Data
Field blocked the wipe, but nothing said so, an admin deleting or
retyping a property saw the same generic success message either way.
Report the kept count back and show a warning naming the conflict, so
there is an actual next step instead of a silent no-op.
…setting

Older datalist datafields were saved without a data_type setting at all,
and the legacy runtime always treated a missing one as string. Migration
read it directly instead, so a fixture like that throws an undefined
array key warning and can abort the whole batch once warnings are
treated as errors.
…e too

Renaming a root property matched stored values by varname only, same as
delete used to. If a legacy Data Field shared that varname its values got
dragged along to the new name. Added renameStoredValues with the same
survivor check deleteStoredValues already has, and wired the form to use it.
--dry-run --delete correctly skipped the actual delete but still printed
the past tense message saying datafields were migrated and deleted. Now it
prints a future tense message and lists what would be removed instead.
…e object

existingCustomProperties and migratedDataFields were private properties
mutated from inside closures, so a method's real output was invisible in
its own signature. prepareCustomProperties and migrateDatafields now return
that data directly, and deleteMigratedDataFields takes it as a parameter.
No behavior change, just less hidden state to track down while reading it.
key_name is varchar(255) with utf8mb4, a full index on it needs more than
767 bytes once you add parent_uuid_v. That is fine on the DYNAMIC row
format everyone gets by default now, but still breaks on a server set up
to use the old COMPACT format on purpose. Capped the index to the first
180 characters so it stays safe either way without touching what a user
can actually store in the column.
@flourish86

Copy link
Copy Markdown
Contributor

An addition to the post above, where the collapsible containers were missing. It would make sense, to also have the top most items collapse.

This would also make sense in the perspective of consistency, same object behave the same, i.e. are collapsible.

This could look like this.

all expanded

Frame 8

some collapsed

Frame 9

Rename used to save first and warn after if a legacy Data Field owned the
old or new name, leaving the property renamed with its values stranded.
Added a key_name validator that checks this up front and blocks the save
with a normal validation error. The model layer still refuses the rename
as a backstop and keeps the old name, in case a Data Field shows up in
the gap between validation and submit.
removeObjectCustomVars() walked host, service, notification, command and
user tables but skipped service_set, unlike the other cleanup methods in
this class. Turns out a service set can hold a stored value through the
REST API even without a UI to attach one, so a nested retype could leave
stale data behind there. Added it to the loop and covered it with a test.
value_type is disabled in the UI once a property is used, but disabled is
just a browser hint, nothing stopped a crafted submission from changing
it server side. That would skip the retype cleanup entirely and leave
old-shape data behind under the new type. Now the stored value_type gets
pinned back whenever the property is in use. Added a test for it, which
needed a small addition to the test form to fake used_count since
assemble() is stubbed out there.
… before save

deleteStoredValues() already refused to wipe values when a legacy Data
Field claimed the same varname, but the form only found out after
saving and warned the admin instead of blocking the change. Added a
value_type validator that checks this up front, the same way the
rename validator does. The model layer still refuses the retype as a
backstop and keeps the old type, in case a Data Field shows up in the
gap between validation and submit.
Same rule the rename check already enforces, applied to new
properties too. No point checking value_type compatibility, just
block it outright like a rename would.
Creating or renaming a Data Field could still collide with an
existing property, the same gap CustomVariableForm just closed from
the other side. Add the matching check and block it the same way.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants